Skip to content

ORC-1950: [C++] Make sure dictionary is sorted before flushed into ORC file to follow ORC specs #2337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
taiyang-li wants to merge 7 commits into from
Closed

ORC-1950: [C++] Make sure dictionary is sorted before flushed into ORC file to follow ORC specs #2337

taiyang-li wants to merge 7 commits into from

Conversation

@taiyang-li
Copy link
Contributor

@taiyang-li taiyang-li commented Jul 22, 2025 •
edited by dongjoon-hyun
Loading

What changes were proposed in this pull request?

Make sure dictionary is sorted before flushed into ORC file to follow ORC specs. The issue was brought by #2336.

Why are the changes needed?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

@github-actions github-actions bot added the CPP label Jul 22, 2025
wgtmac
wgtmac reviewed Jul 22, 2025
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix! I've left some comments.

@github-actions github-actions bot added the BUILD label Jul 22, 2025
wgtmac
wgtmac reviewed Jul 22, 2025
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. Thanks!

c++/src/meson.build
'orc',
sources: source_files,
cpp_args: [
'-DBUILD_SPARSEHASH'
Copy link
Member

@wgtmac wgtmac Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd Could you help review this? If we depend on the sparsehash library in the meson build, what should we do on the Apache Arrow side?

ffacs
ffacs reviewed Jul 22, 2025
View reviewed changes
Copy link
Contributor

@ffacs ffacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taiyang-li Thank you for the fix, could you please add a test to make sure dictionary is ordered?

@taiyang-li
Copy link
Contributor Author

taiyang-li commented Jul 22, 2025

@taiyang-li Thank you for the fix, could you please add a test to make sure dictionary is ordered?

Of course.

@taiyang-li
Copy link
Contributor Author

taiyang-li commented Jul 23, 2025

@taiyang-li Thank you for the fix, could you please add a test to make sure dictionary is ordered?

done

@dongjoon-hyun dongjoon-hyun added this to the 2.2.0 milestone Jul 24, 2025
dongjoon-hyun
dongjoon-hyun approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @taiyang-li and all.

cc @williamhyun

williamhyun
williamhyun approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@williamhyun williamhyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM, will merge this now.

williamhyun pushed a commit that referenced this pull request Jul 24, 2025
@taiyang-li @williamhyun
ORC-1950: [C++] Make sure dictionary is sorted before flushed into OR…
b88dbf5 
…C file to follow ORC specs

### What changes were proposed in this pull request?

Make sure dictionary is sorted before flushed into ORC file to follow ORC specs. The [issue](#2321 (comment)) was brought by #2336.

### Why are the changes needed?

### How was this patch tested?

### Was this patch authored or co-authored using generative AI tooling?

Closes #2337 from taiyang-li/make_dict_sorted.

Authored-by: taiyang-li <654010905@qq.com>
Signed-off-by: William Hyun <william@apache.org>
(cherry picked from commit a4ff1c8)
Signed-off-by: William Hyun <william@apache.org>
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jul 25, 2025

Thank you, @williamhyun .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmac wgtmac wgtmac approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

@ffacs ffacs ffacs approved these changes

@williamhyun williamhyun williamhyun approved these changes

Assignees

No one assigned

Labels

BUILD CPP

Projects

None yet

Milestone

2.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@taiyang-li @dongjoon-hyun @wgtmac @ffacs @williamhyun